-
Notifications
You must be signed in to change notification settings - Fork 117
Convert createActionManager #723
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
b085d50 to
9fc366b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is var?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it wasn't necessary? you can patchAndCleanup the session I guess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not necessary anymore (?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove (everywhere as well) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove ? sorry
8085ebd to
839095b
Compare
839095b to
d6679fd
Compare
aab-odoo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is globally OK, except for the translation part. I'm not sure to understand everything, and I believe it is unnecessary complicated. Let's talk about it tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // We do want to signal willUnmount to the widget and its children | |
| // without prejudice to exportState's comment. | |
| /** | |
| * The ComponentAdapter calls `on_attach_callback` on `this.widget`. However, | |
| * in the ActionAdapter, we unset `this.widget` (see @exportState) to keep the child | |
| * widget alive when this component is unmounted but the controller is still in the stack. | |
| * This override ensures that `this.widget` is set when willUnmount is called on the | |
| * ComponentAdapter. | |
| * | |
| * @override | |
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only used in tests, right? if so, should not be here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
legacyTranslationService
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we sure that this is patched before being called?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not sure this really needs to be so complicated
7ce2048 to
91f3de6
Compare
692e1f5 to
6a0ab41
Compare
9dcb549 to
f566bac
Compare
|
Sorry, I didn't know about this PR and had to retrieve its information, you may have to re-approve it. |
804c7dd to
d5b301b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to destroy (done via registerCleanup in createWebClient)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you try not to prettify all this file?
5151e20 to
0de8c87
Compare
0de8c87 to
b818ceb
Compare
|
robodoo r+ |
|
Linked pull request(s) odoo-dev/enterprise#140 not ready. Linked PRs are not staged until all of them are ready. |
|
Because this PR has multiple commits, I need to know how to merge it:
|
|
robodoo rebase-ff |
|
Merge method set to rebase and fast-forward |
|
robodoo retry |
|
I'm sorry, @kebeclibre. Retry makes no sense when the PR is not in error. |
closes #723 Signed-off-by: Aaron Bohy (aab) <aab@odoo.com>
No description provided.